Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

Merged
merged 15 commits into from
Aug 3, 2023

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Jul 31, 2023

Overview

Closes ##2138
This PR also addresses the concerns raised in the comment by introducing a distinction between two types of reserved namespaces. Namespaces within the range [0-255] are now identified as "Primary Reserved Namespaces," while any other reserved namespaces beyond this range are categorized as "Non-Primary Reserved Namespaces." Hopefully, this differentiation provides better clarity and organization regarding namespace allocation and usage within the code.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@staheri14 staheri14 self-assigned this Aug 1, 2023
@staheri14 staheri14 changed the title feat: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function Aug 1, 2023
@staheri14 staheri14 added this to the Mainnet milestone Aug 1, 2023
@staheri14 staheri14 marked this pull request as ready for review August 1, 2023 14:42
@staheri14 staheri14 added warn:api breaking item will be break an API and require a major bump and removed warn:api breaking item will be break an API and require a major bump labels Aug 1, 2023
@staheri14 staheri14 enabled auto-merge (squash) August 1, 2023 15:01
rootulp
rootulp previously approved these changes Aug 1, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I like the addition of "primary". Should we also update it in the specs?

| `MAX_RESERVED_NAMESPACE` | `Namespace` | `0x00000000000000000000000000000000000000000000000000000000FF` | Max reserved namespace is lexicographically the largest namespace that is reserved for protocol use. |

pkg/namespace/namespace.go Outdated Show resolved Hide resolved
x/blob/types/payforblob_test.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-2194/
on branch gh-pages at 2023-08-03 08:18 UTC

@staheri14 staheri14 force-pushed the sanaz/extends-is-reserved-namespace branch from d09f934 to 7c76ff6 Compare August 2, 2023 08:01
@staheri14
Copy link
Contributor Author

LGTM, I like the addition of "primary". Should we also update it in the specs?

Good point, please see the updates on the specs in bbffdf3 and 7c76ff6.

@staheri14 staheri14 requested a review from rootulp August 2, 2023 08:03
evan-forbes
evan-forbes previously approved these changes Aug 2, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

cmwaters
cmwaters previously approved these changes Aug 2, 2023
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change

In addition to the items listed in this table, it should be noted that namespaces with values less than `0x00000000000000000000000000000000000000000000000000000000FF` are exclusively reserved for use within the Celestia protocols.
Reserved namespaces fall into two categories, _Primary Reserved Namespaces_ and _Secondary Reserved Namespaces_.
- Primary Reserved Namespaces: Namespaces with values less than or equal to `0x00000000000000000000000000000000000000000000000000000000FF` are referred to as Primary Reserved Namespace and are exclusively reserved for use within the Celestia protocols.
- Secondary Reserved Namespaces: Currently, there are two secondary reserved namespaces, namely, `PARITY_SHARE_NAMESPACE` and `TAIL_PADDING_NAMESPACE` which fall out of the range of primary reserved namespaces.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever want to give ourselves some padding so we can add secondary namespaces in the future? The same we do with primary?

}

if ns.IsParityShares() {
return ErrParitySharesNamespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these errors used anywhere else? Do we want to remove them if they aren't used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After integrating this PR, there will be no active usage for ErrParitySharesNamespace in the app. I also initially thought of deleting it, however, there are other unused errors among the existing registered errors e.g., ErrInvalidShareCommitments, so I decided to keep it as is. @rootulp Do you think there will be future usage for this error?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect future usages of ErrParitySharesNamespace so it seems feasible to remove but we can def do in a follow-up issue / PR.

One question I have is: when we delete errors that are no longer used, do we need to reserve the error code indefinitely or can it be repurposed?

Copy link
Member

@rach-id rach-id Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, sometimes some errors are not used but kept so that their code number is not replaced by some other error which might break downstream for people depending on it

Edit: Rootul front ran me :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too sure who uses the error code since the message is always accompanied with it in the response but I'd tend to agree that safest thing to do would be to reserve it (just by leaving a comment probably)

rootulp
rootulp previously approved these changes Aug 2, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the specs change! Thanks for pushing this through

specs/src/specs/namespace.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md Outdated Show resolved Hide resolved
}

if ns.IsParityShares() {
return ErrParitySharesNamespace
Copy link
Member

@rach-id rach-id Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, sometimes some errors are not used but kept so that their code number is not replaced by some other error which might break downstream for people depending on it

Edit: Rootul front ran me :D

@rootulp rootulp dismissed stale reviews from cmwaters, evan-forbes, and themself via d3b29c8 August 2, 2023 14:32
@MSevey MSevey requested a review from a team August 2, 2023 14:33
@codecov-commenter
Copy link

Codecov Report

Merging #2194 (c0cf2cd) into main (9ea27d4) will decrease coverage by 0.03%.
The diff coverage is 20.00%.

❗ Current head c0cf2cd differs from pull request most recent head b9fdc3c. Consider uploading reports for the commit b9fdc3c to get more accurate results

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
- Coverage   24.89%   24.86%   -0.03%     
==========================================
  Files         127      127              
  Lines       14335    14326       -9     
==========================================
- Hits         3568     3562       -6     
+ Misses      10400    10397       -3     
  Partials      367      367              
Files Changed Coverage Δ
pkg/namespace/namespace.go 64.48% <0.00%> (-1.87%) ⬇️
pkg/namespace/random_blob.go 0.00% <ø> (ø)
x/blob/types/payforblob.go 75.70% <100.00%> (-0.67%) ⬇️

@staheri14 staheri14 merged commit b3b8611 into main Aug 3, 2023
27 checks passed
@staheri14 staheri14 deleted the sanaz/extends-is-reserved-namespace branch August 3, 2023 13:12
rootulp pushed a commit that referenced this pull request Aug 14, 2023
…to IsReserved function (#2194)

Closes ##2138
This PR also addresses the concerns raised in the
[comment](#2138 (comment))
by introducing a distinction between two types of reserved namespaces.
Namespaces within the range [0-255] are now identified as "Primary
Reserved Namespaces," while any other reserved namespaces beyond this
range are categorized as "Non-Primary Reserved Namespaces." Hopefully,
this differentiation provides better clarity and organization regarding
namespace allocation and usage within the code.

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
@evan-forbes evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Sep 5, 2023
mergify bot pushed a commit that referenced this pull request Sep 5, 2023
…to IsReserved function (#2194)

## Overview
Closes ##2138
This PR also addresses the concerns raised in the
[comment](#2138 (comment))
by introducing a distinction between two types of reserved namespaces.
Namespaces within the range [0-255] are now identified as "Primary
Reserved Namespaces," while any other reserved namespaces beyond this
range are categorized as "Non-Primary Reserved Namespaces." Hopefully,
this differentiation provides better clarity and organization regarding
namespace allocation and usage within the code.

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

(cherry picked from commit b3b8611)

# Conflicts:
#	pkg/namespace/consts.go
#	pkg/namespace/namespace.go
#	specs/src/specs/namespace.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorporate ParitySharesNamespace and TailPaddingNamespace into IsReserved function
6 participants